Matchinfo lifetime #1225

zachs18 · 2023-11-13T21:45:29Z

May fix #1223

Adds a (covariant) lifetime parameter to glib::MatchInfo, and restricts the glib::Regex methods that return MatchInfo to taking only &'input GStr as input (instead of impl IntoGStr) and returning MatchInfo<'input> that borrows from the input. Also MatchInfo::string now returns a borrowed &'input GStr instead of an owned GString.

Manually implements the MatchInfo struct and related traits since the wrapper! macro does not support lifetime generics (I used rust-analyzer's "expand macro recursively" option on the previous autogenerated impl using the wrapper! macro, copy-pasted it, and modified it as needed.)

Removes the ValueType/ToValue/FromValue impls from MatchInfo as they would be unsound (could be used to "extend" the borrowed lifetime; see glib/tests/regex_compiletest/04-no-to-value.rs). Also removes the HasParamSpec impl for MatchInfo¹.

Relaxes the ToGlibPtr<'a, *mut T> for Shared<T, MM> impl's lifetime bound to only bound MM: 'a instead of MM: 'static so that MatchInfo can still use it. The methods in this trait return raw pointers, and the methods in the FromGlibPtr trait which take these pointers is unsafe, so I believe this should be fine.

Restricts the ValueType/ToValue/HasParamSpec impls to MatchInfo<'static>. FromValue<'a> is implemented for MatchInfo for all lifetimes (longer than 'a).

Manually implements the reference counting instead of using glib::shared::Shared, since otherwise caused issues with variance².

I removed the HasParamSpec impl for MatchInfo mostly because it "seemed" wrong; I haven't come up with a concrete example of unsoundness it could cause without FromValue/ToValue also being implemented. ↩
(Shared<_, MM> is invariant in MM, meaning holding a Shared<_, Self> in MatchInfo<'input> causes MatchInfo to be invariant in 'input) ↩

sdroege

Thanks, looks mostly good to me :)

sdroege · 2023-11-14T07:24:02Z

glib/src/match_info.rs

+#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
+#[repr(transparent)]
+pub struct MatchInfo<'input> {
+    inner: crate::shared::Shared<ffi::GMatchInfo, Self>,


Suggested change

inner: crate::shared::Shared<ffi::GMatchInfo, Self>,

inner: std::ptr::NonNull<ffi::GMatchInfo>,

should be sufficient or not? Then the SharedMemoryManager noise below can also go away

That would work too, but would require manually implementing the reference-counting logic that Shared already implements, i.e. calling g_match_info_ref in clone instead of deriving Clone, and implementing Drop to call g_match_info_unref.

I guess the question then is which is simpler. Your call

I went with using NonNull and manually implementing the reference counting, since Shared also caused issues with variance (Shared<_, MM> is invariant in MM, meaning holding a Shared<_, Self> in MatchInfo<'input> causes MatchInfo to be invariant in 'input, when ideally it should be covariant).

glib/src/match_info.rs

sdroege · 2023-11-14T07:28:27Z

glib/src/shared.rs

@@ -527,7 +527,7 @@ impl<T, MM: SharedMemoryManager<Target = T>> Hash for Shared<T, MM> {

 impl<'a, T: 'static, MM> ToGlibPtr<'a, *mut T> for Shared<T, MM>
 where
-    MM: SharedMemoryManager<Target = T> + 'static,
+    MM: SharedMemoryManager<Target = T> + 'a,


This should probably go into a separate commit (in the same PR). Is the same change needed for src/boxed.rs too, for example?

right, we need that for gtk-rs/gtk4-rs#1280

That bug is basically the same thing as this one here, yes

This change is no longer part of this PR, since I now implement the reference counting manually instead of using SharedMemoryManager/Shared<P, MM> due to variance (#1225 (comment))

zachs18 · 2023-11-17T04:07:00Z

Okay, now that MatchInfo<'input> is actually covariant in 'input I think this is done.
I also added back the ValueType/ToValue/HasParamSpec impls, but only for MatchInfo<'static>. FromValue<'a> is implemented for MatchInfo<'_> for any lifetime longer than 'a, since it's fine to convert a MatchInfo<'long> to MatchInfo<'short>. These impls allow having MatchInfo<'static> as a property (see glib/tests/regex_compiletest/06-property.rs)
Foreign code could set a MatchInfo<'static> property to a GMatchInfo that isn't actually 'static, but using foreign code is unsafe anyway, and as long as the MatchInfo isn't used after the input string, it's sound for the lifetime to "lie".

sdroege · 2023-11-17T08:57:15Z

glib/src/match_info.rs

+    type BuilderFn = fn(&str) -> crate::ParamSpecBoxedBuilder<Self>;
+
+    fn param_spec_builder() -> Self::BuilderFn {
+        |name| Self::ParamSpec::builder(name)


error: redundant closure --> glib/src/match_info.rs:267:9 | 267 | |name| Self::ParamSpec::builder(name) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `Self::ParamSpec::builder` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure = note: `-D clippy::redundant-closure` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::redundant_closure)]`

sdroege

Thanks, looks good to me. Can you squash it all into a single commit with a meaningful commit message? Then I'll merge it :)

Implement MatchInfo manually. Manually implements refcounting and from/into glib ptrs instead of using crate::shared::Shared, since using Shared<_, Self> causes the lifetime parameter to be invariant instead of covariant. Removes array/slice conversions for `MatchInfo`. ValueType/ToValue/HasParamSpec impls are restricted to `MatchInfo<'static>` to avoid use-after-free by converting `MatchInfo<'short>` to `Value`. FromValue is implemented for MatchInfo for any lifetime, since MatchInfo is covariant anyway.

zachs18 mentioned this pull request Nov 13, 2023

[BUG] Current (unstable) glib::Regex::match_* API is unsound (use-after-free) #1223

Closed

sdroege reviewed Nov 14, 2023

View reviewed changes

zachs18 force-pushed the matchinfo-lifetime branch from 617fa8e to 8abb00d Compare November 14, 2023 19:27

sdroege previously approved these changes Nov 15, 2023

View reviewed changes

zachs18 dismissed sdroege’s stale review via fe601ef November 17, 2023 03:51

zachs18 force-pushed the matchinfo-lifetime branch from 8abb00d to fe601ef Compare November 17, 2023 03:51

sdroege reviewed Nov 17, 2023

View reviewed changes

sdroege approved these changes Nov 17, 2023

View reviewed changes

zachs18 force-pushed the matchinfo-lifetime branch from ea24437 to 2fd6046 Compare November 17, 2023 18:59

sdroege merged commit 72b073c into gtk-rs:master Nov 19, 2023
38 of 48 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Matchinfo lifetime #1225

Matchinfo lifetime #1225

zachs18 commented Nov 13, 2023 •

edited

Loading

sdroege left a comment

sdroege Nov 14, 2023

zachs18 Nov 14, 2023

sdroege Nov 15, 2023

zachs18 Nov 17, 2023

sdroege Nov 14, 2023

bilelmoussaoui Nov 14, 2023

sdroege Nov 14, 2023

zachs18 Nov 17, 2023

zachs18 commented Nov 17, 2023 •

edited

Loading

sdroege Nov 17, 2023

sdroege left a comment

	inner: crate::shared::Shared<ffi::GMatchInfo, Self>,
	inner: std::ptr::NonNull<ffi::GMatchInfo>,

Matchinfo lifetime #1225

Matchinfo lifetime #1225

Conversation

zachs18 commented Nov 13, 2023 • edited Loading

Footnotes

sdroege left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachs18 commented Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

sdroege left a comment

Choose a reason for hiding this comment

zachs18 commented Nov 13, 2023 •

edited

Loading

zachs18 commented Nov 17, 2023 •

edited

Loading